Skip to content

Conversation

@kacpermuda
Copy link
Contributor

generate_openlineage_events_from_dbt_cloud_run assumed the OL provider has some features that were introduced in 2.0.0 version. Added an explicit requirement on the function, so that it throws AirflowOptionalProviderFeatureException if the OL provider version is not enough.

As the function checking the provider version is in common.compat, I needed to add compat provider as requirement for DBT cloud provider.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@kacpermuda kacpermuda force-pushed the fix-ol-dbt-add-required-version branch 2 times, most recently from 531adb9 to 8595198 Compare March 20, 2025 14:11
@potiuk
Copy link
Member

potiuk commented Mar 22, 2025

Nice. But I think we should make an explicit bump as a separate PR in common.compat. This is something that we should start doing to keep the common.compat more of a managed "what's in which version".

I think #47909 was a bit wrong in the sense that it did not bump common.compat version to 1.6.0 - such bump should happen there, because that's where the functionality was added, not here where the functionality is "used".

We can rectify by extracting common.compat version bump to a separate PR from this one.

We have no good mechanisms for full automation here (yet - maybe we should introduce it at some point of time), but for now I think it could be relatively easy thing to follow a process similar to this:

  1. Make a PR that changes both common.compat - including major version bump - and the provider to use the new feature and make it green (do not merge - it should be draft).

  2. Split out the PR part for common.compat - where both "common.compat" change and "minor version bump" are done.
    We should also handle the case where we already have an unreleased `common.compat" minor version bumped and we want to add "another" functionality to it - then we do not need to bump the minor version. That part is a bit tricky, because we need to somewhere have the information "common.compat already had minor version bumped but it is not yet relased". I have no good idea yet how to store that information in an easy way to know it without going to PyPI and checking (also being aware that we are just voting new providers including commopn.compate which is a bit of an edge case). Merge it first.

  3. Rebase the original PR (effectively removing the common.compat change from it) and merging only changes related to the provider that needs new "common.compat") - dbt in this case.

WDYT. @kacpermuda @mobuchowski @eladkal ? Maybe you have some other ideas here and maybe we should write-down a small description on how we approach common.compat changes?

@mobuchowski
Copy link
Contributor

@potiuk your suggestions make sense to me.

I have no good idea yet how to store that information in an easy way to know it without going to PyPI and checking

I think for now we need to rely on contributors (and possibly reviewers) checking it manually... maybe this should live in common providers documentation?

@kacpermuda kacpermuda force-pushed the fix-ol-dbt-add-required-version branch from 8595198 to 587881f Compare March 27, 2025 12:54
@kacpermuda
Copy link
Contributor Author

@potiuk The general idea sounds good to me, I'm not sure I follow on how exactly you'd expect it to be done, but I'm eager to make it work. I think this time Elad already bumped the version and prepared the rc1, so I have no need to bum,p the common.compat anymore, but for the next feature I'll make sure to follow this approach. If this makes sense, let me know if we should merge this one then?

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

@potiuk potiuk merged commit 0afaa5d into apache:main Mar 31, 2025
148 checks passed
@kacpermuda kacpermuda deleted the fix-ol-dbt-add-required-version branch March 31, 2025 07:31
simonprydden pushed a commit to simonprydden/airflow that referenced this pull request Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants